SEP-1875: Warn when a reused sep service account's Grafana role is below Admin by peter-o-addo · Pull Request #1428 · percona/SEP

peter-o-addo · 2026-08-31T13:02:15Z

Summary

Warn when a reused sep Grafana service account has an org role below Admin by probing the freshly minted token.

sidecar/grafana_service_account.py: thread (id, created) from find_or_create_account and expose reused from mint/mint_with_retry; probe reused accounts after mint and warn on FORBIDDEN, with the same UNREACHABLE stderr diagnostic as keep_persisted_token; share the role-gap message via warn_role_gap(subject); document the probe path on resolve_token
changelog.d/SEP-1875.changed.md: note the new stderr warning for under-privileged reused accounts
tests/sidecar/test_grafana_service_account.py: cover reuse/create mint returns, post-mint probe warn/skip paths (FORBIDDEN, UNREACHABLE, healthy Admin), race-recovery reuse, and that VALIDATE uses the minted Bearer token

Tested

Minting onto a reused Viewer account writes the Admin role warning and still returns the token
Creating a new account mints a token with no role warning
Minting onto a reused Admin account returns a token with no role warning

Checklist

New/modified functions have type hints and rST docstrings
New tests added for new features or bug fixes
All tests pass locally (make test)
Pre-commit hooks pass (make run-pre-commit)
Database migrations generated if models changed (make makemigrations)
User-facing changes documented (README, inline help, UI text)
Configuration changes documented with examples
Changelog fragment added under changelog.d/ if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change, or a same-release-cycle fix for an unreleased sibling ticket)

Copilot

🟡 Changes recommended

The user-visible warning needs a changelog fragment, and one new integration test duplicates an existing path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Tracks whether a Grafana service account was reused, then warns when its token lacks Admin privileges.

Changes:

Return reuse metadata from account/token minting.
Probe tokens minted for reused accounts.
Add coverage for creation, reuse, and race-recovery paths.

File summaries

File	Description
`sidecar/grafana_service_account.py`	Adds reuse tracking and role warning.
`tests/sidecar/test_grafana_service_account.py`	Tests mint results and post-mint probes.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🔵 Needs a closer look

The core test does not verify that the freshly minted token is actually used for the role probe.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

sidecar/grafana_service_account.py:305

Keep this docstring scoped to this helper’s return contract. Explaining how callers use the flag couples these reference docs to resolve_token, so the text will become stale if probing moves or another caller is added.

This issue also appears on line 531 of the same file.

    The second element is ``True`` when the id came from a search (initial or
    race-recovery) rather than a create this call performed. Callers that mint
    a token onto a reused account can then probe it for an org role below
    ``Admin``, which this create path cannot leave behind.

tests/sidecar/test_grafana_service_account.py:942

The queued 403 is returned regardless of which credential the probe sends, so this test would still pass if resolve_token accidentally validated with the admin credentials or another token. Assert the recorded Authorization header here to prove the newly minted token is the one being probed.

    assert grafana_stub.calls(StubRoute.VALIDATE)

sidecar/grafana_service_account.py:534

resolve_token now has a new observable warning path, but its docstring still describes only token resolution. Document the reused-account probe and warning so the function contract stays synchronized with this behavior change.

            token, reused = await mint_with_retry(
                provider, admin_credentials(), deadline
            )
            if reused:

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Balanced

Copilot

🟢 Approval recommended

The implementation is focused, correctly documented, and adequately tested across relevant paths.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Balanced

peter-o-addo · 2026-08-31T15:07:42Z

A local Grafana 11.2 container was started with admin/admin, confirmed healthy on :3000, and a temp working directory was prepared with the sidecar settings.yaml plus empty SEP_STATE_DIR for the mint runs below.

Step 1 — Scenario 1: reused Viewer `sep` (expect warning)

1a — Delete any existing `sep` service account

Deleted any existing Grafana service account named exactly sep via the admin search/delete API.

Deleted sep account id=2

1b — Create `sep` with org role Viewer

Created a new sep service account with org role Viewer so the mint path would reuse an under-privileged account.

Created sep id=3 role=Viewer

1c — Clear persisted token and run the mint helper

Cleared $SEP_STATE_DIR/grafana_service_account_token and ran sidecar/grafana_service_account.py against that Grafana with no configured token so it would mint onto the reused Viewer account.

exit=0
stdout_token_chars=46
stderr:
  [grafana-mint] Grafana accepts the freshly minted token but the 'sep' service account ranks below Admin in its org; a re-mint would carry the same role, so the token is kept.
persisted=yes

Expect: exit 0, token on stdout, ranks below Admin on stderr, token persisted.

Step 2 — Scenario 2: fresh create (expect no role-gap warning)

2a — Delete the Viewer `sep` account

Deleted the Viewer sep service account so the next mint would take the create path.

Deleted sep account id=3

2b — Clear persisted token and run the mint helper

Cleared the persisted token again and ran the mint helper so Grafana created a fresh Admin sep account with no prior reuse.

exit=0
stdout_token_chars=46
stderr:
persisted=yes

Expect: exit 0, token on stdout, empty stderr (no ranks below), token persisted. Grafana now has an Admin sep account created by the helper.

Step 3 — Scenario 3: reused Admin `sep` (expect no role-gap warning)

3a — Clear persisted token only (keep the Admin `sep` account)

Removed only the persisted token file while leaving the Admin sep account in place so the next mint would reuse it.

3b — Run the mint helper again

Ran the mint helper again so it minted onto the existing Admin sep account and probed without a role-gap warning.

exit=0
stdout_token_chars=46
stderr:
persisted=yes

Expect: exit 0, token on stdout, no ranks below warning (probe ACCEPTED), token persisted.

github-actions · 2026-08-31T15:15:31Z

Coverage report

This PR does not seem to contain any modification to coverable code.

yyyyyyyan

@peter-o-addo — the reuse flag is threaded at the source rather than re-derived in resolve_token, so both reuse branches (grafana_service_account.py:315, :329) carry it and only the create path at :337 opts out. That is the right place for the "don't probe a fresh account" carve-out to live. test_a_reused_account_at_admin_probes_without_warning is also the test this shape usually skips: it pins the negative case and still asserts calls(StubRoute.VALIDATE)[0], so it cannot pass vacuously if the probe never fired.

Mint-path probe drops the sibling's UNREACHABLE diagnostic — sidecar/grafana_service_account.py:539. keep_persisted_token splits the same validate_token result two ways, FORBIDDEN at :495 and UNREACHABLE at :502; the new probe handles only the first. A Grafana that goes unreachable in the window between the mint and the probe then costs up to PROBE_TIMEOUT_SECONDS of silent wait with nothing on stderr naming why — the same silence this ticket set out to remove.

One smaller note on the duplicated warning text is inline.

Approving — the findings above are yours to take or leave; resolve the threads when you have.

peter-o-addo · 2026-09-01T14:44:59Z

@yyyyyyyan , All three threads are addressed, UNREACHABLE on the mint probe, warn_role_gap(subject), and (id, created) on find_or_create_account. Could you take another look?

yyyyyyyan

@peter-o-addo — all three points from the last round are addressed cleanly. The mint-path probe now mirrors keep_persisted_token on UNREACHABLE, with test_a_reused_account_with_unreachable_probe_warns_after_mint pinning the diagnostic; warn_role_gap collapses the duplicated role-gap message to a single site; and find_or_create_account reads as (id, created) with the negation confined to mint.

Approving — nothing outstanding from my side.

peter-o-addo added 6 commits August 31, 2026 15:39

Report reuse vs create from find_or_create_account

1d6d994

Propagate account reuse flag through mint

f46ed17

Warn when a reused sep account's role is below Admin

822136d

Update mint tests for reuse flag return

7bb458a

Add tests for mint-path probe on reused SEP accounts

2f86b84

Add tests for healthy reused mint probes without warnings

ed902f1

github-actions Bot added the python label Aug 31, 2026

github-actions Bot assigned peter-o-addo Aug 31, 2026

peter-o-addo marked this pull request as ready for review August 31, 2026 13:04

peter-o-addo requested a review from a team as a code owner August 31, 2026 13:04

Copilot AI balanced review requested due to automatic review settings August 31, 2026 13:04

peter-o-addo requested review from marcuscruz-percona and yyyyyyyan as code owners August 31, 2026 13:04

Copilot started reviewing on behalf of peter-o-addo August 31, 2026 13:05 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread sidecar/grafana_service_account.py Outdated

Comment thread tests/sidecar/test_grafana_service_account.py Outdated

peter-o-addo added 2 commits August 31, 2026 16:35

Add tests for create-path probe skip on first-start mint

22b3c3c

Add changelog

9b4862a

peter-o-addo requested a review from a team as a code owner August 31, 2026 13:35

peter-o-addo requested a balanced review from Copilot August 31, 2026 13:37

Copilot started reviewing on behalf of peter-o-addo August 31, 2026 13:37 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

peter-o-addo added 2 commits August 31, 2026 16:45

Add tests for post-mint probe using the minted token

8c53ffb

Sync mint-path docstrings with reuse probe behavior

4919a2f

peter-o-addo requested a balanced review from Copilot August 31, 2026 13:46

Copilot started reviewing on behalf of peter-o-addo August 31, 2026 13:47 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

peter-o-addo added the qa in progress Someone is currently testing this PR - do not merge it label Aug 31, 2026

peter-o-addo added qa passed Tests for this PR are completed and successful. and removed qa in progress Someone is currently testing this PR - do not merge it labels Aug 31, 2026

yyyyyyyan approved these changes Aug 31, 2026

View reviewed changes

Comment thread sidecar/grafana_service_account.py

Comment thread sidecar/grafana_service_account.py

yyyyyyyan requested changes Aug 31, 2026

View reviewed changes

Comment thread sidecar/grafana_service_account.py

peter-o-addo added 4 commits September 1, 2026 13:43

Fix missing UNREACHABLE warning on mint-path token probe

d5b55c0

Align find_or_create_account with get_or_create convention

7665141

Share role-gap warning via warn_role_gap helper

4e88ed1

Merge branch 'main' into SEP-1875

ebae57c

peter-o-addo requested a review from yyyyyyyan September 1, 2026 14:45

yyyyyyyan approved these changes Sep 2, 2026

View reviewed changes

Merge branch 'main' into SEP-1875

d5b9aa6

peter-o-addo enabled auto-merge (squash) September 3, 2026 13:49

peter-o-addo merged commit 6973ffe into main Sep 3, 2026
89 of 103 checks passed

peter-o-addo deleted the SEP-1875 branch September 3, 2026 15:40

Navigation Menu

Uh oh!

SEP-1875: Warn when a reused sep service account's Grafana role is below Admin - #1428

SEP-1875: Warn when a reused sep service account's Grafana role is below Admin#1428
peter-o-addo merged 15 commits into
mainfrom
SEP-1875

peter-o-addo commented Aug 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

peter-o-addo commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

yyyyyyyan left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peter-o-addo commented Sep 1, 2026

Uh oh!

yyyyyyyan left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

peter-o-addo commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tested

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (3)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

peter-o-addo commented Aug 31, 2026

Step 1 — Scenario 1: reused Viewer sep (expect warning)

1a — Delete any existing sep service account

1b — Create sep with org role Viewer

1c — Clear persisted token and run the mint helper

Step 2 — Scenario 2: fresh create (expect no role-gap warning)

2a — Delete the Viewer sep account

2b — Clear persisted token and run the mint helper

Step 3 — Scenario 3: reused Admin sep (expect no role-gap warning)

3a — Clear persisted token only (keep the Admin sep account)

3b — Run the mint helper again

Uh oh!

github-actions Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Uh oh!

yyyyyyyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peter-o-addo commented Sep 1, 2026

Uh oh!

yyyyyyyan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peter-o-addo commented Aug 31, 2026 •

edited

Loading

Step 1 — Scenario 1: reused Viewer `sep` (expect warning)

1a — Delete any existing `sep` service account

1b — Create `sep` with org role Viewer

2a — Delete the Viewer `sep` account

Step 3 — Scenario 3: reused Admin `sep` (expect no role-gap warning)

3a — Clear persisted token only (keep the Admin `sep` account)

github-actions Bot commented Aug 31, 2026 •

edited

Loading